Java: add more Spring RestTemplate request forgery sinks #20930
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends SSRF (Server-Side Request Forgery) detection for Spring RestTemplate to include all methods with uriVariables parameters, not just getForObject. This addresses customer feedback about enterprise code usage patterns that weren't previously covered.
Key changes:
- Refactored specific
getForObjectsink detection to a generic approach covering allRestTemplatemethods withuriVariablesparameters - Added comprehensive test coverage for all 11 affected methods (
delete,exchange,execute,getForEntity,getForObject,headForHeaders,optionsForAllow,patchForObject,postForEntity,postForLocation,postForObject,put) - Updated expected test results to reflect the expanded sink detection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll |
Refactored sink detection from method-specific to generic class that handles all RestTemplate methods with uriVariables parameters; added method position mapping |
java/ql/test/query-tests/security/CWE-918/SpringSSRF.java |
Added comprehensive test cases for all 11 RestTemplate methods with various URL patterns and variable substitution scenarios |
java/ql/test/query-tests/security/CWE-918/RequestForgery.expected |
Updated expected results to include new alerts from expanded RestTemplate method coverage |
java/ql/lib/change-notes/2025-11-27-spring-rest-template-request-forgery-sinks.md |
Added change note documenting the expansion of SSRF sink detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Outdated
Show resolved
Hide resolved
| private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink { | ||
| SpringWebClientRestTemplateGetForObject() { |
There was a problem hiding this comment.
The class name SpringWebClientRestTemplateGetForObject is misleading since it now handles all RestTemplate methods with uriVariables parameters, not just getForObject. Consider renaming to something more general like SpringWebClientRestTemplateUriVariables to better reflect its purpose.
| private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink { | |
| SpringWebClientRestTemplateGetForObject() { | |
| private class SpringWebClientRestTemplateUriVariables extends RequestForgerySink { | |
| SpringWebClientRestTemplateUriVariables() { |
| exists(SpringRestTemplateGetForObjectMethodCall mc, int i | | ||
| exists(SpringRestTemplateMethodWithUriVariablesParameter m, MethodCall mc, int i | | ||
| // Note that the first argument is modeled as a request forgery sink | ||
| // separately. This model is for arguments beyond the first two. There |
There was a problem hiding this comment.
The comment "arguments beyond the first two" is inaccurate for some methods. For example, delete has uriVariables at position 1, making it "arguments beyond the first", and headForHeaders/optionsForAllow also have uriVariables at position 1. Consider updating to "arguments beyond the URL parameter" or a similar generic description.
| // separately. This model is for arguments beyond the first two. There | |
| // separately. This model is for arguments beyond the URL parameter (i.e., those corresponding to `uriVariables`). There |
Co-authored-by: Copilot <[email protected]>
1 participant
This is an extension of #18153 to include all the other methods on the class
RestTemplatewhich have a parameter nameduriVariables. They should all be request forgery sinks, but the original PR only did it forgetForObject.It is hard to do performance analysis or evaluate precision because they are very few uses of these APIs in public repos. (We know from customer feedback that they are used in enterprise code.) I looked at the tuple counts from running the query on the test and didn't see any sign of bad join orders.